fix(update): harden npm cache recovery preflight logs by lidge-jun · Pull Request #557 · lidge-jun/opencodex

lidge-jun · 2026-07-27T14:52:27Z

Summary

Maintainer takeover follow-up for #533. This keeps the contributor PR unmerged and addresses the two high-severity review blockers from the latest Wibias review:

fail closed when the npm cache is same-UID but not effectively readable/searchable/writable before any proxy stop path runs
sanitize persisted update job command/log/error fields so installer output does not retain raw home/cache paths or uid/gid values

The medium policy question around nonzero installer recovery behavior is intentionally left for maintainer review; this PR is draft and should not be merged automatically.

Verification

bun test tests/update-npm-cache-preflight.test.ts tests/update-job.test.ts → 70 pass / 0 fail
bun test tests/update-npm-cache-preflight.test.ts tests/update-stop-first.test.ts tests/update-job.test.ts tests/update-install-process.test.ts tests/config.test.ts tests/ocx-launcher-source.test.ts tests/windows-deploy-close-regressions.test.ts → 162 pass / 0 fail
bun x tsc --noEmit → pass
git diff --check origin/dev...HEAD → pass
bun run privacy:scan → pass
pre-push bun scripts/test.ts → 5127 pass / 0 fail
pre-push React Doctor changed scope → no issues found

Refs #533.

coderabbitai · 2026-07-27T14:52:34Z

Important

Review skipped

Draft detected.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: dbe26b3b-2269-4239-96ad-578488ac10e3

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

🔍 Trigger review

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch codex/pr533-update-recovery-hardening

_{Comment @coderabbitai help to get the list of available commands.}

WZBbiao · 2026-07-28T02:28:20Z

Thanks for taking over this change. I agree that #557 should be the merge vehicle.

For the remaining nonzero-installer policy, I recommend keeping the current fail-closed behavior and documenting the manual recovery path. Without reliable descendant containment, automatically starting a recovery package could race with an installer process that is still mutating the global package tree.

Once #557 is ready and merged, #533 can be closed as superseded. Please also retain attribution to #533 and @WZBbiao in the PR description and final merge commit. The imported commits currently preserve the original wzb author identity, but GitHub does not link that email to the WZBbiao account.

WP1 docs-only cycle. Five decade docs at diff-level precision, ordered by dependency (auth gate -> response layer -> adapter -> PR cleanup) rather than effort: - 010 SSH remote proxy: isLoopbackRequestHost couples loopback identity to port equality, so ssh -L on a different local port 403s the whole /v1/* data plane. The sibling isLoopbackOriginValue already dropped its port check in e4e0612 for the same reason. - 020 issue lidge-jun#553: the same three-line 'Provider unreachable' shape repeats at core.ts 1197/1746/1788; fold into one helper and give ERR_TLS_CERT_ALTNAME_INVALID its own wording plus a verification command. - 030 issue lidge-jun#545: the anthropic adapter prepends the Claude Code identity unconditionally on OAuth, so a classifier that already carries it loses output budget it capped at 64 tokens and retries. - 040 PR lidge-jun#527: its first commit already landed on dev as 9dd3c42, which is what makes the branch dirty; rebase keeping only a64aa58. - 050 PR lidge-jun#557: no code work remains, only a security-boundary decision. No production code changed in this cycle.

lidge-jun · 2026-07-30T14:06:39Z

NEEDS-CHANGES, then security review — one of the two takeover blockers is still open.

The effective-access check is done properly. src/update/npm-cache-preflight.mjs:98-113 calls accessSync with read/write/search on directories and read/write on files, which covers the ACL, read-only, and mode cases that a same-UID ownership check misses. tests/update-npm-cache-preflight.test.ts:117-165 injects EACCES and proves the preflight fails closed before either stop path. That blocker is closed.

The log sanitizer is not. The path regexes at src/update/job.ts:324-330 all use [^\s"'<>], so they terminate at the first space. I ran the exact regexes from this branch against realistic inputs:

C:\Users\Alice Smith\.npm\_cacache\tmp\entry   ->  unchanged, fully leaked
C:\Users\AliceSmith\.npm\_cacache\tmp\entry    ->  [redacted npm path]
/Users/Alice Smith/.npm/_cacache/tmp           ->  [redacted user path] Smith[redacted npm path]
/home/alice/.npm/_cacache/tmp                  ->  [redacted npm path]

A Windows profile with a space in the name — which is the common case, since that is what Windows generates from a full name — survives completely unredacted into persisted update-job state. The Unix form leaks the surname. The requirement was to keep raw home and cache paths and usernames out of persisted fields, and that is not met.

Worse, tests/update-job.test.ts:113-145 only covers /home/alice/... with no space, so the suite is green while the defect is live. A test that passes over the one input shape that works is more dangerous than no test.

Fix: allow spaces inside the path body while still terminating on a real delimiter (quote, angle bracket, newline, or end of string), and add regressions for C:\Users\Alice Smith\... and /Users/Alice Smith/....

On the rebase. The six conflicting files are not mechanical. In bin/ocx.mjs and src/update/index.ts, current dev's trusted absolute npmInvocation meets this branch's tree-aware runner head-on; in src/update/job.ts the stale-worker handling meets the PID-identity APIs. Taking either side wholesale drops security behaviour that the other side added. These need to be integrated, not chosen between.

On #533. git range-diff against the correct merge bases pairs all 14 of its commits with the first 14 here and marks only d215cbeb9 and b0434ea58 as new, so this really is the strict superset and the sole merge vehicle. Nobody should be asking both branches to rebase the same 3,000 lines. I have said the same on #533 and left attribution intact.

This changes dependency installation, executable resolution, process cleanup, persisted installer output, and recovery execution, so MAINTAINERS.md:33-34 requires explicit security review. Green CI does not substitute for it.

dev CI is fully green now, so checks here will reflect this branch.

lidge-jun · 2026-07-30T14:22:37Z

Blocking — maintainer note in place of a formal review.

GitHub refuses request-changes on your own pull request, so this comment carries the same weight as the CHANGES_REQUESTED reviews now on the rest of the queue. Treating it as blocking.

The open item is the log sanitizer. src/update/job.ts:324-330 terminates every path regex at whitespace, so a Windows profile with a space in the name survives completely unredacted into persisted update-job state, and the Unix form leaks the surname. tests/update-job.test.ts:113-145 only covers the space-free case, so the suite is green over a live defect.

Until that is fixed this PR cannot be the merge vehicle for #533, which means #533 also stays open. Those two move together.

Everything else in the takeover is in good shape — the accessSync effective-access check closes its blocker properly, and the fail-closed preflight regression proves it.

wzb and others added 15 commits July 27, 2026 23:43

fix(update): preserve proxy on npm cache failures

e332d22

fix(update): harden cache preflight and recovery

7efed6c

fix(update): close failed-install recovery gaps

6db4dc7

fix(update): harden preflight and recovery races

5c35865

fix(update): consolidate replacement pid checks

55d72e6

fix(update): revalidate restart pid identity

f1f75fa

fix(update): close failed-install recovery races

e3a837e

fix(update): harden failed-install recovery

07f0748

fix(update): close final recovery trust gaps

ea27540

fix(update): harden CLI installer cleanup

5564b03

fix(update): close installer recovery review gaps

1fdc443

fix(update): address recovery review feedback

a9daa12

fix(update): harden recovery scan boundaries

eb9f89f

fix(update): close recovery review gaps

8dfe46d

fix(update): harden npm recovery preflight logs

d215cbe

github-actions Bot added the bug Something isn't working label Jul 27, 2026

lidge-jun mentioned this pull request Jul 27, 2026

fix(update): preserve proxy on npm cache failures #533

Closed

3 tasks

test(update): assert launcher suffix with platform separator

b0434ea

luvs01 mentioned this pull request Jul 30, 2026

fix(update): tolerate late proxy readiness #737

Merged

3 tasks

lidge-jun mentioned this pull request Jul 30, 2026

Tracking: release queue, security review, and open blockers #755

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(update): harden npm cache recovery preflight logs - #557

fix(update): harden npm cache recovery preflight logs#557
lidge-jun wants to merge 16 commits into
devfrom
codex/pr533-update-recovery-hardening

lidge-jun commented Jul 27, 2026

Uh oh!

coderabbitai Bot commented Jul 27, 2026 •

edited

Loading

Review skipped

Uh oh!

WZBbiao commented Jul 28, 2026

Uh oh!

lidge-jun commented Jul 30, 2026

Uh oh!

lidge-jun commented Jul 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

lidge-jun commented Jul 27, 2026

Summary

Verification

Uh oh!

coderabbitai Bot commented Jul 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review skipped

Uh oh!

WZBbiao commented Jul 28, 2026

Uh oh!

lidge-jun commented Jul 30, 2026

Uh oh!

lidge-jun commented Jul 30, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

coderabbitai Bot commented Jul 27, 2026 •

edited

Loading